Skip to content

[EXIF] Support UCS2 and 8-byte encoded string tags #1935

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 38 commits into from
Feb 3, 2022
Merged

[EXIF] Support UCS2 and 8-byte encoded string tags #1935

merged 38 commits into from
Feb 3, 2022

Conversation

@IldarKhayrutdinov
Copy link
Contributor

@IldarKhayrutdinov IldarKhayrutdinov commented Jan 10, 2022 •
edited
Loading

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Fixes for #1919

0x9c9b 40091 Image Exif.Image.XPTitle Byte Title tag used by Windows, encoded in UCS2
0x9c9c 40092 Image Exif.Image.XPComment Byte Comment tag used by Windows, encoded in UCS2
0x9c9d 40093 Image Exif.Image.XPAuthor Byte Author tag used by Windows, encoded in UCS2
0x9c9e 40094 Image Exif.Image.XPKeywords Byte Keywords tag used by Windows, encoded in UCS2
0x9c9f 40095 Image Exif.Image.XPSubject Byte Subject tag used by Windows, encoded in UCS2

See: https://www.exiv2.org/tags.html

Todo

  • Review API
  • Add tests
  • Review UCS-2 encoded tags

@IldarKhayrutdinov IldarKhayrutdinov mentioned this pull request Jan 10, 2022
Closed
4 tasks
gfoidl
gfoidl reviewed Jan 11, 2022
View reviewed changes
@IldarKhayrutdinov IldarKhayrutdinov marked this pull request as ready for review January 19, 2022 09:53
@IldarKhayrutdinov IldarKhayrutdinov marked this pull request as draft January 19, 2022 09:56
@IldarKhayrutdinov
Copy link
Contributor Author

IldarKhayrutdinov commented Jan 19, 2022

@JimBobSquarePants Codecov is not working

@JimBobSquarePants
Copy link
Member

JimBobSquarePants commented Jan 20, 2022

@JimBobSquarePants Codecov is not working

Yeah I turned it off for PRs in #1934 because the build/test times were prohibitively expensive. It now runs on a schedule

IldarKhayrutdinov
IldarKhayrutdinov commented Jan 30, 2022
View reviewed changes
@IldarKhayrutdinov IldarKhayrutdinov marked this pull request as ready for review January 30, 2022 13:27
@IldarKhayrutdinov IldarKhayrutdinov changed the title EXIF Encoded strings [EXIF] Support UCS2 and 8-byte encoded string tags Jan 30, 2022
@IldarKhayrutdinov
Copy link
Contributor Author

IldarKhayrutdinov commented Jan 30, 2022 •
edited
Loading

Add new test. The JIS write and read tests pass successfully, but for some reason ExifTool cannot correctly display the JIS encoded text.

ExifTool output:

ExifTool Version Number         : 12.39
File Name                       : Calliphora_encoded_strings.jpg
Directory                       : .
File Size                       : 252 KiB
File Modification Date/Time     : 2022:01:30 17:50:40+03:00
File Access Date/Time           : 2022:01:30 17:58:59+03:00
File Creation Date/Time         : 2022:01:06 11:42:08+03:00
File Permissions                : -rw-rw-rw-
File Type                       : JPEG
File Type Extension             : jpg
MIME Type                       : image/jpeg
JFIF Version                    : 1.01
Resolution Unit                 : inches
X Resolution                    : 96
Y Resolution                    : 96
Exif Byte Order                 : Little-endian (Intel, II)
XP Title                        : A bit of test metadata for image title
XP Comment                      : A bit of test metadata for image comment
XP Author                       : Dan Petitt
XP Keywords                     : Keyword1;Keyword2
XP Subject                      : This is a subject
User Comment                    : 箖望춂낐嚓좂沸喇
GPS Date Stamp                  : 2022:01:06
GPS Processing Method           : GPS processing method (ASCII)
GPS Area Information            : GPS area info (Unicode)
Image Width                     : 804
Image Height                    : 1198
Encoding Process                : Baseline DCT, Huffman coding
Bits Per Sample                 : 8
Color Components                : 3
Y Cb Cr Sub Sampling            : YCbCr4:4:4 (1 1)
Image Size                      : 804x1198
Megapixels                      : 0.963

@IldarKhayrutdinov
Copy link
Contributor Author

IldarKhayrutdinov commented Jan 30, 2022 •
edited
Loading

I change JIS encoding, though Shift JIS standardized as JIS X 0208 Appendix 1 and popular encoding. By docs for JIS 0208-1990 needs to use 20932 code.

Test updated with english text. WIndows show correctly, but ExifTool still incorrect.
image

JimBobSquarePants
JimBobSquarePants reviewed Jan 30, 2022
View reviewed changes
src/ImageSharp/Metadata/Profiles/Exif/ExifEncodedStringHelpers.cs Outdated
(uint)GetEncoding(encodedString.Code).GetByteCount(encodedString.Text) + CharacterCodeBytesLength;

public static bool IsEncodedString(ExifTagValue tag)
public static byte[] GetData(EncodedString encodedString)
Copy link
Member

@JimBobSquarePants JimBobSquarePants Jan 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used only by the writer yeah? If so I would return an IDisposable containing a pooled buffer which you could use to avoid the buffer allocation.

Copy link
Contributor Author

@IldarKhayrutdinov IldarKhayrutdinov Feb 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

directly writing to output buffer 5d50298

Copy link
Member

@JimBobSquarePants JimBobSquarePants Feb 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ace, thanks! I'll review this today.

JimBobSquarePants
JimBobSquarePants approved these changes Feb 3, 2022
View reviewed changes
Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks excellent. Thanks so much for helping out with the fix! 👍

I had a good read of the reference material you provided regard the JIS encoding and the one you have chosen does match the specification. I've unlocked the spec PDF to make this more easily searchable.

@JimBobSquarePants JimBobSquarePants merged commit ea38d98 into SixLabors:master Feb 3, 2022
@JimBobSquarePants JimBobSquarePants added this to the 2.0.0 milestone Feb 3, 2022
@IldarKhayrutdinov IldarKhayrutdinov deleted the exif-encoded-strings branch February 3, 2022 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JimBobSquarePants JimBobSquarePants JimBobSquarePants approved these changes

+1 more reviewer

@gfoidl gfoidl gfoidl left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug metadata:exif

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@IldarKhayrutdinov @JimBobSquarePants @gfoidl